Skip to content

Expand Salesforce sync to SchoolClass, ClassTeacher, and Lesson#851

Merged
fspeirs merged 9 commits into
mainfrom
fs-expand-salesforce-sync
Jun 9, 2026
Merged

Expand Salesforce sync to SchoolClass, ClassTeacher, and Lesson#851
fspeirs merged 9 commits into
mainfrom
fs-expand-salesforce-sync

Conversation

@fspeirs

@fspeirs fspeirs commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Related to https://github.com/RaspberryPiFoundation/experience-cs/issues/2142

Summary

Extends the Heroku Connect Salesforce sync pattern from #677 to three more Salesforce objects:

editor-api model Salesforce object Fields synced
SchoolClass Classroom__c identifiers, name, member count, parent Editor__c lookup
ClassTeacher Contact_Classroom_Affiliation__c identifiers, parent Classroom__c and Contact lookups
Lesson (classroom-bound) Lesson__c identifiers, name, teacher-project metadata, assigned/completed counts, parent Classroom__c lookup

It also generalises the parent-sync race guard that #850 introduced for RoleSyncJob so every sync job using __r__<external_id> lookups gets it for free, and documents the operational shape of the sync layer in AGENTS.md.

Behaviour

Triggers. after_commit on each source model enqueues a per-record GoodJob (Salesforce::{SchoolClass,Lesson,ClassTeacher}SyncJob), gated on FeatureFlags.salesforce_sync?. Per-record concurrency on the base job collapses duplicate enqueues from same-transaction commits (e.g. new class + initial teacher).

Lesson completion is two flows. Lesson__c.numberofcompletedprojects__c is the sum of:

  • Lesson#submitted_projects_count — Statesman-driven, used by the Code Editor (Python/HTML) flow when a SchoolProject transitions to :submitted. Unchanged from main.
  • Lesson#finished_projects_count — live read of school_projects.where(finished: true).count, used by Experience CS for Scratch projects (which flip finished directly and never enter the state machine).

The two flows are mutually exclusive in practice, so summing is safe. SchoolProject gets a new after_commit on saved_change_to_finished? to enqueue LessonSyncJob when Experience CS flips the flag.

Assignment count is event-driven. Lesson__c.numberofassignedprojects__c is lesson.remixes.count, which only changes when a remix Project is created. The enqueue lives on Project after_commit :create (remixed_from_id.present?), not on roster changes — so adding a student to a class no longer scans every lesson in the class.

Parent-sync race guard. ensure_parent_synced!(model, external_id_field, external_id, label) on Salesforce::SalesforceSyncJob checks the parent has a non-nil sfid in its Heroku Connect mirror and raises SalesforceRecordNotFound if not. The base job retries with polynomial backoff, so jobs self-heal once the parent lands instead of leaving the child row permanently FAILED in Heroku Connect. This is a generalisation of the mechanism introduced in #850.

Backfill

After deploying with SALESFORCE_ENABLED=true, run the backfill rake tasks in this order so each child's parent already has an sfid in its mirror by the time the child enqueues:

rails salesforce_sync:school
rails salesforce_sync:role
rails salesforce_sync:contact
rails salesforce_sync:school_class
rails salesforce_sync:class_teacher
rails salesforce_sync:lesson

Each task is Model.find_each { perform_later } — non-blocking; the worker drains the salesforce_sync queue at concurrency 10. Watch progress at /admin/good_job.

Strict ordering isn't load-bearing: ensure_parent_synced! defers any child whose parent hasn't landed via retry_on SalesforceRecordNotFound, wait: :polynomially_longer, attempts: 10, so out-of-order runs self-heal. Running parents first just cuts retry churn.

Re-running is idempotent — each job uses find_or_initialize_by on the external-ID key, so repeats update in place rather than duplicating rows.

Out of scope

  • Destroy semantics for mirror rows. This PR doesn't delete or soft-delete mirror rows when the source record is destroyed (existing behaviour). Worth deciding before GA.
  • Observability on race-guard exhaustion. No Sentry alert when retry_on SalesforceRecordNotFound exhausts its attempts.
  • salesforce_sync:all convenience task to run the canonical backfill order in one go.

Test plan

  • docker compose run --rm api rspec spec/jobs/salesforce spec/models/{lesson,school_class,school_project_salesforce_sync,class_teacher_salesforce_sync,class_student_salesforce_sync,project_salesforce_sync}_spec.rb
  • docker compose run --rm api bundle exec rubocop
  • After deploy with SALESFORCE_ENABLED=true, run the backfill tasks per the Backfill section above.
  • In salesforce_connect, confirm every parent row has a non-nil sfid before child writes and that any transient _hc_err rows self-heal via the retry.

Made with Cursor

@cla-bot cla-bot Bot added the cla-signed label Jun 5, 2026
@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown

Test coverage

91.46% line coverage reported by SimpleCov.
Run: https://github.com/RaspberryPiFoundation/editor-api/actions/runs/27195605305

fspeirs and others added 6 commits June 8, 2026 12:10
The Heroku Connect parent-sync race guard introduced for RoleSyncJob in
#850 applies to every sync job that uses __r__ external-ID lookups to
resolve a Salesforce parent. Move it onto the base class so SchoolClass,
ClassTeacher, and Lesson sync jobs (added in this branch) can call it
without duplicating the helper or its rationale.

No behaviour change: RoleSyncJob still raises the same
SalesforceRecordNotFound and is still retried by the base-class
retry_on; the method now just lives one inheritance hop away.

Co-authored-by: Cursor <cursoragent@cursor.com>
These ActiveRecord models map onto the Heroku Connect-managed tables
(classroom__c, lesson__c, contact_classroom_affiliation__c) in the
salesforce_connect database. Each declares its external-ID field, hides
the Heroku-Connect-only sfid / _hc_lastop / _hc_err columns, and exposes
Salesforce field accessors derived from the editor-api parent record so
sync jobs can assemble payloads without leaking column names.

Factories included for use in the upcoming sync-job specs.

Pure data layer; nothing in the app references these models yet.

Co-authored-by: Cursor <cursoragent@cursor.com>
Three new GoodJob-backed sync jobs follow the established
SalesforceSyncJob pattern (mapped attributes + truncation + concurrency
keyed on the source record id):

  Salesforce::SchoolClassSyncJob   -> Classroom__c
  Salesforce::ClassTeacherSyncJob  -> Contact_Classroom_Affiliation__c
  Salesforce::LessonSyncJob        -> Lesson__c

Each calls ensure_parent_synced! before saving so the new __r__
external-ID lookups can't permanently FAIL on a parent that hasn't
landed yet; the base class retry handles redelivery.

Adds matching rake tasks (salesforce_sync:school_class,
salesforce_sync:class_teacher, salesforce_sync:lesson) so backfill stays
consistent with the existing school/role/contact pattern.

Includes a shared not_have_enqueued_job negated matcher in spec/support
for use by the upcoming callback specs.

No model callbacks yet -- jobs are wired up in the next commit.

Co-authored-by: Cursor <cursoragent@cursor.com>
Hook the new sync jobs onto the editor-api model graph. Each callback is
guarded by FeatureFlags.salesforce_sync? so dev/test stay quiet by
default.

  SchoolClass             -> SchoolClassSyncJob on create/update
  Lesson                  -> LessonSyncJob on create/update (class-bound)
  ClassTeacher            -> ClassTeacherSyncJob on create;
                             SchoolClassSyncJob on create/destroy
                             (refreshes numberofmembers__c)
  ClassStudent            -> SchoolClassSyncJob on create/destroy
                             (numberofmembers__c only -- no per-lesson
                             cascade; remix Project creation drives
                             lesson syncs instead)
  Project (remix)         -> LessonSyncJob on create
                             (numberofassignedprojects__c is exactly
                             remixes.count, which only changes here)
  SchoolProject (state)   -> recalculate submitted_projects_count and
                             enqueue LessonSyncJob on transitions
                             into/out of :submitted and :complete

Brand-new classes race SchoolClassSyncJob vs. ClassTeacherSyncJob; the
race is harmless because ensure_parent_synced! defers the child write
until the parent lands and per-record concurrency on the base job
collapses duplicate enqueues.

Includes model specs (school_class, lesson, class_teacher,
class_student, project) and the negated_matchers helper to assert
"on enqueue X and explicitly not Y" with compound expectations.

Co-authored-by: Cursor <cursoragent@cursor.com>
Scratch projects are the primary focus of this Salesforce work but they
don't transition through the SchoolProject state machine -- Experience
CS marks them complete by flipping school_projects.finished via
Concept::SchoolProject::SetFinished. Without a separate path the
Statesman-driven submitted_projects_count would miss every Scratch
completion and Lesson__c.numberofcompletedprojects__c would be 0 for
any Scratch-only lesson.

  * Lesson#finished_projects_count: live read of school_projects.where(
    finished: true).count. Not cached because its only consumer is the
    per-lesson concurrency-limited LessonSyncJob; the recalc-on-write
    machinery used for submitted_projects_count would be overkill here.

  * LessonSyncJob: numberofcompletedprojects__c now sums
    submitted_projects_count + finished_projects_count. The two flows
    are mutually exclusive in practice, so a plain sum is safe.

  * SchoolProject after_commit on saved_change_to_finished?: enqueue
    LessonSyncJob so the Salesforce mirror updates when Experience CS
    flips the flag. Statesman-driven transitions remain handled by the
    state-machine after_transition callback wired in the previous
    commit.

Recalculation of submitted_projects_count is unchanged: it still counts
only Statesman :submitted rows, keeping the two flows independent.

Co-authored-by: Cursor <cursoragent@cursor.com>
Capture the operational shape of the sync layer for future agents:
where data goes (salesforce_connect DB, not the Salesforce API), how
it's gated (SALESFORCE_ENABLED), the backfill ordering, and -- most
importantly -- the parent-sync race guard that every job using __r__
external-ID lookups has to call before writing.

Heroku Connect's permanent-FAIL behaviour on missing parents is a
non-obvious failure mode; the doc names ensure_parent_synced!, points
at the retry policy on the base class, and lists the call sites so the
pattern stays consistent as more sync jobs are added.

Co-authored-by: Cursor <cursoragent@cursor.com>
@fspeirs fspeirs force-pushed the fs-expand-salesforce-sync branch from 0625d55 to f57968b Compare June 8, 2026 11:14
@fspeirs fspeirs marked this pull request as ready for review June 8, 2026 12:43
Copilot AI review requested due to automatic review settings June 8, 2026 12:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Extends the existing Heroku Connect–backed Salesforce sync layer to cover classroom-related entities (SchoolClass/ClassTeacher/Lesson) and adds event-driven triggers so Salesforce mirror rows stay up-to-date as roster, assignment, and completion signals change.

Changes:

  • Added new Salesforce mirror models and sync jobs for SchoolClass, ClassTeacher, and classroom-bound Lesson, with parent-sync race protection via SalesforceSyncJob#ensure_parent_synced!.
  • Added/updated model callbacks and event triggers (Project remix creation, SchoolProject finished changes, classroom roster changes) to enqueue the appropriate sync jobs.
  • Added rake tasks, factories, and RSpec coverage for the new sync surface (including parent-not-synced retry behavior), plus small test helper support for compound negated matchers.

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
spec/support/negated_matchers.rb Adds a negated job-enqueue matcher for compound RSpec expectations.
spec/models/school_project_salesforce_sync_spec.rb Verifies Lesson sync job enqueued when SchoolProject.finished flips.
spec/models/school_class_spec.rb Adds model-level expectations for SchoolClass Salesforce sync enqueues.
spec/models/project_salesforce_sync_spec.rb Verifies Lesson sync job enqueued on remix Project creation.
spec/models/lesson_spec.rb Adds specs for finished_projects_count and Lesson Salesforce sync enqueue behavior.
spec/models/class_teacher_salesforce_sync_spec.rb Verifies enqueues for classroom + affiliation sync on create/destroy.
spec/models/class_student_salesforce_sync_spec.rb Verifies roster change enqueues classroom sync but not lesson sync.
spec/jobs/salesforce/school_class_sync_job_spec.rb Covers field mappings, member count, retry guard, and disablement for SchoolClass sync.
spec/jobs/salesforce/lesson_sync_job_spec.rb Covers Lesson field mappings, assigned/completed counts, retry guard, and disablement.
spec/jobs/salesforce/class_teacher_sync_job_spec.rb Covers affiliation mappings, retry guard for both parents, and disablement.
spec/factories/salesforce/school_class.rb Adds FactoryBot factory for Salesforce SchoolClass mirror rows.
spec/factories/salesforce/lesson.rb Adds FactoryBot factory for Salesforce Lesson mirror rows.
spec/factories/salesforce/class_teacher.rb Adds FactoryBot factory for Salesforce ClassTeacher mirror rows.
lib/tasks/salesforce_sync.rake Adds backfill tasks for school classes, class teachers, and classroom lessons.
app/state_machines/school_project_state_machine.rb Adds Salesforce Lesson sync enqueues on certain state transitions.
app/models/school_project.rb Enqueues Lesson sync when Experience CS flips finished.
app/models/school_class.rb Enqueues SchoolClass sync on create/update.
app/models/salesforce/school_class.rb Adds AR model for salesforce.classroom__c.
app/models/salesforce/lesson.rb Adds AR model for salesforce.lesson__c.
app/models/salesforce/class_teacher.rb Adds AR model for salesforce.contact_classroom_affiliation__c.
app/models/project.rb Enqueues Lesson sync when a remix is created (assignment count freshness).
app/models/lesson.rb Adds classroom-only after_commit sync and finished_projects_count.
app/models/class_teacher.rb Enqueues classroom sync on create/destroy and affiliation sync on create.
app/models/class_student.rb Enqueues classroom sync on roster create/destroy.
app/jobs/salesforce/school_class_sync_job.rb Implements SchoolClass mirror upsert with parent-sync guard.
app/jobs/salesforce/salesforce_sync_job.rb Generalizes parent-sync race guard helper into base class.
app/jobs/salesforce/role_sync_job.rb Removes now-redundant local parent-sync guard implementation.
app/jobs/salesforce/lesson_sync_job.rb Implements Lesson mirror upsert incl. assigned/completed counts + parent guard.
app/jobs/salesforce/class_teacher_sync_job.rb Implements ClassTeacher mirror upsert with dual parent guards.
AGENTS.md Documents Salesforce sync operational expectations and backfill ordering.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/state_machines/school_project_state_machine.rb Outdated
Comment thread app/models/lesson.rb Outdated
Comment on lines +47 to +54
# Counts Experience CS Scratch completions for this lesson. Experience CS marks a
# project complete by flipping school_projects.finished via Concept::SchoolProject::SetFinished,
# bypassing the Statesman state machine entirely — so submitted_projects_count never
# sees these. Read live (no cached column) because the only consumer is LessonSyncJob,
# which is per-lesson concurrency-limited; if a non-sync reader ever needs this, add a
# cached column then. Summed with submitted_projects_count in LessonSyncJob to derive
# Lesson__c.numberofcompletedprojects__c.
def finished_projects_count

@zetter-rpf zetter-rpf Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is a bit of an information overload and not sure how helpful it is.

  • why would a non-sync reader need to use a cached column? I would expect this to be doing a single database query, what's the problem with that?
  • I think it's unusual to list callers within documentation - if this changes then the comments not going to be kept in date.

Instead of this comment, I wondered if there's a way to make it clearer that this is for experience CS projects - for example just calling it finished_experience_cs_scratch_project_count would make that clear and maybe make the comment uneeded.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did consider putting some reference to experience_cs in the method name, but decided against it on the grounds that finished is a first-class schema field in editor-api, which leads to a bit of necessary explanation in the comment (I agree that it can be simplified and I will).

Let me know what you think - I'm happy to use your function name if you think it will be clearer going forward.

Comment thread app/jobs/salesforce/lesson_sync_job.rb Outdated
mapped_attributes(lesson:).merge(
teacherprojecttitle__c: lesson.project&.name,
teacherprojecttype__c: lesson.project&.project_type,
numberofassignedprojects__c: lesson.remixes.count,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is counting remixes a good way to tell us assigned projects?

Remixes count might be a good approximation for started projects, but it wouldn't tell you if a teacher has assigned a project. As far as I know we don't model project assignment, we just know if a project has been made available to a student in a class.

This might be a naming thing - maybe numberofassignedprojects__c should be called 'number of started/saved projects' or similar

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it's a terminology issue, maybe. The assumption is that if the project is assigned to the class, it is assigned to all the students in the class.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe it could be removed if the existing class member count is sufficient?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, I mis-remembered how that part worked. What counts as an "assigned" project is when the student has first saved their remix of a teacher's project.

@zetter-rpf zetter-rpf Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw you added a comment to the code, but I'm unsure about the reasoning behind this - what do the salesforce team want to track - project assignments or project starts?

If it's project assignments, shouldn't use the class member count (or remove this since we already have that?). if it's project starts, could this be renamed? Or is this terminology already used for reporting Experience CS metrics?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this on Slack but, for the GitHub record, we decided that the actual metric is "projects visible to students in the classroom" so in commit aca52ff we changed it to this calculation.

Comment thread app/models/class_teacher.rb
@zetter-rpf

Copy link
Copy Markdown
Contributor

I'm finding some of the comments in app/models files hard to understand, I think it's because they are terminology that's not in our codebase already (like 'roster', or 'parent classroom mirror'). I think it's also because they leak some of the details out of the salesforce implementation into the models.

Do you think these comments are important? I wasn't sure if you've taken the time to write them or they were autogenerated. Some of these might be better documented in the PR or commit history.

Comment thread spec/support/negated_matchers.rb
Drop the redundant `enqueue_salesforce_lesson_sync` after_transition callbacks
on `SchoolProjectStateMachine`. State-machine transitions that change a synced
field already fan out via `Lesson#recalculate_submitted_projects_count!` →
`Lesson#after_commit :do_salesforce_sync`, so the explicit enqueues either
duplicated existing jobs (collapsed by per-record concurrency) or were no-ops
for transitions where nothing synced changes. Lock the new behaviour with
specs covering both the "exactly once" path (via the recalc) and the
"don't enqueue" path (unsubmitted ↔ complete).

Rewrite hook comments on `ClassTeacher`, `ClassStudent`, `Project`,
`SchoolProject`, and `Lesson` in codebase terms — drop "mirror", "roster",
and `__c` field names that leaked Heroku-Connect / Salesforce vocabulary
into model files. Add a one-line note at the `numberofassignedprojects__c`
call site flagging the SF field-name mismatch (counts started, not assigned).

Co-authored-by: Cursor <cursoragent@cursor.com>
@fspeirs

fspeirs commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

On the question of leaking Salesforce into the models, I somewhat agree. One of my concerns here, that I rather ran out of time to consider doing, is that this work layers a network of event-driven sync jobs, where the dependencies between them might be rather non-obvious to future developers.

…count

Replace the remix-based approximation with the new semantic: a lesson is
"assigned" to every student in its class iff its visibility is set to
'students'. The SF field name now matches its meaning, resolving the
naming concern raised in PR #851 review.

Reshape the sync triggers around the new dependency graph:
- LessonSyncJob computes `lesson.school_class.students.count` when
  visibility == 'students', else 0.
- ClassStudent create/destroy fans out to every visible-to-students
  lesson in the class. The fanout queries via the FK
  (`Lesson.where(school_class_id: ...)`) rather than the `school_class`
  association so it stays safe under cascading destroy: when a
  SchoolClass is destroyed, its students cascade and the after_commit
  fires after the parent row is gone, at which point the association
  resolves to nil. The FK on the in-memory destroyed record stays
  readable, and the lessons it once owned are nullified or deleted, so
  the relation is empty in the cascade case and correct otherwise.
- Project#after_commit on remix create is removed — remix creation no
  longer affects any synced field on Lesson__c (numberofcompletedprojects__c
  is driven by SchoolProject state/finished changes; numberofassignedprojects__c
  no longer depends on remix count).

Backfill: run `rails salesforce_sync:lesson` after deploy to refresh
existing rows with the new count.

Trade-off accepted: roster changes now enqueue N+1 jobs per ClassStudent
event (one SchoolClassSyncJob plus one LessonSyncJob per student-visible
lesson). Bounded fanout, collapsed by per-record concurrency on the base
job for rapid duplicates.

Loss to flag: Salesforce no longer carries a "students who have started"
signal. If reporting wants that back, the cleanest path is a separate
SF field fed by a restored Project#after_commit on remix create.

Co-authored-by: Cursor <cursoragent@cursor.com>
@fspeirs fspeirs temporarily deployed to editor-api-p-fs-expand--5u8ns0 June 9, 2026 09:03 Inactive
@zetter-rpf

Copy link
Copy Markdown
Contributor

Great, thanks for the improvements

@fspeirs fspeirs merged commit c1ab020 into main Jun 9, 2026
5 checks passed
@fspeirs fspeirs deleted the fs-expand-salesforce-sync branch June 9, 2026 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants